Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughWidened Python compatibility and CI/test targets: pyproject metadata updated to allow Python <3.15 and add 3.14 classifier; CI workflow matrix and setup updated from 3.13→3.14; one integration test class is skipped on Python ≥3.14. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #840 +/- ##
==========================================
- Coverage 94.10% 94.05% -0.05%
==========================================
Files 39 39
Lines 2053 2053
==========================================
- Hits 1932 1931 -1
- Misses 121 122 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
Member
Author
|
Member
Author
|
Member
Author
|
@liamhuber Somehow the integration tests fail with Python 3.14. It seems to be related to the use of def dynamic_foo():
"""
A decorator for dynamically modifying the Foo class to test
CloudpickleProcessPoolExecutor.
Overrides the `fnc` input of `Foo` with the decorated function.
"""
def as_dynamic_foo(fnc: Callable):
return type(
"DynamicFoo",
(Foo,), # Define parentage
{"__init__": partialmethod(Foo.__init__, fnc)},
)
return as_dynamic_fooI was wondering if you had similar issues with |
Member
Author
|
It seems to be a |
Member
|
I haven't tested 3.14 for workflow yet, so I'm not sure. I've done it only(?) for snippets, but that is pretty trivial. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/pipeline.yml (2)
192-221:unittest_flux_mpichandunittest_flux_openmpiintentionally left on 3.13.Keeping flux-related jobs on 3.13 appears to be a deliberate decision (likely due to
flux-core=0.81.0and related conda packages not being available for Python 3.14). Worth adding a comment in the YAML to document why these differ from the other test jobs, to avoid confusion during future version bumps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pipeline.yml around lines 192 - 221, Add an inline comment above the unittest_flux_mpich and unittest_flux_openmpi job definitions explaining why they pin python-version: '3.13' (e.g., flux-core=0.81.0 and related conda packages are not available on Python 3.14), so future maintainers know the version divergence is intentional; place the comment near the job headers (unittest_flux_mpich / unittest_flux_openmpi) and mention the dependency (flux-core=0.81.0) and that the pin is to satisfy conda package compatibility.
44-58:mypyjob still on Python 3.13 — consider whether 3.14 should be added here too.The
mypyjob at Line 51 is the only type-checking job; it still runs on 3.13. While not blocking, any type-checking against 3.14-only syntax or stub differences won't be caught. If this is intentional (no 3.14-specific type changes expected), no action needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pipeline.yml around lines 44 - 58, Update the mypy job to also run under Python 3.14 so type-checking covers 3.14-specific syntax/stubs: modify the mypy job (the job named "mypy" and its "Setup Python" step that uses actions/setup-python@v5 and the python-version input) to either use a matrix over python-version including "3.13" and "3.14", or add a parallel mypy job configured with python-version: "3.14"; ensure the subsequent steps (Checkout, Install mypy, Test) run for both Python versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/test_pyiron_workflow.py`:
- Around line 59-62: The skip condition incorrectly uses sys.version_info.minor
twice; change the decorator's condition to compare the version tuple instead of
comparing .minor fields (e.g., replace the current sys.version_info.minor >= 14
and sys.version_info.minor >= 3 with a tuple comparison like sys.version_info >=
(3, 14) or an equivalent major/minor check) so the `@unittest.skipIf` using
sys.version_info correctly skips on Python versions >= 3.14; update the
`@unittest.skipIf` expression that references sys.version_info in the
test_pyiron_workflow.py decorator accordingly.
---
Nitpick comments:
In @.github/workflows/pipeline.yml:
- Around line 192-221: Add an inline comment above the unittest_flux_mpich and
unittest_flux_openmpi job definitions explaining why they pin python-version:
'3.13' (e.g., flux-core=0.81.0 and related conda packages are not available on
Python 3.14), so future maintainers know the version divergence is intentional;
place the comment near the job headers (unittest_flux_mpich /
unittest_flux_openmpi) and mention the dependency (flux-core=0.81.0) and that
the pin is to satisfy conda package compatibility.
- Around line 44-58: Update the mypy job to also run under Python 3.14 so
type-checking covers 3.14-specific syntax/stubs: modify the mypy job (the job
named "mypy" and its "Setup Python" step that uses actions/setup-python@v5 and
the python-version input) to either use a matrix over python-version including
"3.13" and "3.14", or add a parallel mypy job configured with python-version:
"3.14"; ensure the subsequent steps (Checkout, Install mypy, Test) run for both
Python versions.
Comment on lines
+59
to
+62
| @unittest.skipIf( | ||
| sys.version_info.minor >= 14 and sys.version_info.minor >= 3, | ||
| "Test environment has to be Python <3.14 for dynamic objects.", | ||
| ) |
Contributor
There was a problem hiding this comment.
Bug in skipIf condition: .minor used twice instead of .major + .minor.
sys.version_info.minor >= 14 and sys.version_info.minor >= 3Both sides reference .minor. The second clause (minor >= 3) is always True when the first (minor >= 14) is, making the and entirely redundant. The condition collapses to sys.version_info.minor >= 14, which happens to work for current Python 3.x releases but would silently misbehave on a future Python 4.x (e.g., 4.0 has minor=0, so no skip even if the same cloudpickle bug persists).
The idiomatic and correct form is a tuple comparison:
🐛 Proposed fix
`@unittest.skipIf`(
- sys.version_info.minor >= 14 and sys.version_info.minor >= 3,
+ sys.version_info >= (3, 14),
"Test environment has to be Python <3.14 for dynamic objects.",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_pyiron_workflow.py` around lines 59 - 62, The skip
condition incorrectly uses sys.version_info.minor twice; change the decorator's
condition to compare the version tuple instead of comparing .minor fields (e.g.,
replace the current sys.version_info.minor >= 14 and sys.version_info.minor >= 3
with a tuple comparison like sys.version_info >= (3, 14) or an equivalent
major/minor check) so the `@unittest.skipIf` using sys.version_info correctly
skips on Python versions >= 3.14; update the `@unittest.skipIf` expression that
references sys.version_info in the test_pyiron_workflow.py decorator
accordingly.
Member
Author
|
Member
Author
|
The minimal tests are using a different Python version: versus: |
Member
Author
|
More issues with threading without global interpreter lock: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit